Add Read(Span<byte>) override to MultipartReaderStream - #69070
Add Read(Span<byte>) override to MultipartReaderStream#69070SimonCropp wants to merge 3 commits into
Conversation
Move the synchronous read implementation onto Read(Span<byte>) and have the array overload delegate to it, so a caller reading a section into a span does not pay the base Stream fallback's rented array and copy. Also add the throwing Write(ReadOnlySpan<byte>) to match the other write overrides.
|
Thanks for your PR, @SimonCropp. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
🟡 Changes recommended
The new span-based sync read path calls into BufferedReadStream.Read(Span<byte>) fallback (rent+copy) and—because Read(byte[],...) now delegates—appears to introduce a performance regression for all sync reads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates MultipartReaderStream’s synchronous read/write surface to add a Read(Span<byte>) override (and have the byte[] overload delegate to it), plus adds a throwing Write(ReadOnlySpan<byte>) override to match the existing write behavior.
Changes:
- Added
Write(ReadOnlySpan<byte>)override that throwsNotSupportedException. - Routed
Read(byte[], int, int)toRead(Span<byte>)and implemented the core sync read logic on the span overload. - Updated internal sync read slices to use span-based reads.
File summaries
| File | Description |
|---|---|
| src/Http/WebUtilities/src/MultipartReaderStream.cs | Moves sync read implementation to Read(Span<byte>) and adds Write(ReadOnlySpan<byte>) throw override. |
Review details
Suppressed comments (1)
src/Http/WebUtilities/src/MultipartReaderStream.cs:176
- The new synchronous
Read(Span<byte>)override isn’t exercised by existing multipart tests (e.g., MultipartReaderTests usesCopyToAsync/ReadAsyncpaths). Since this change affects the sync read implementation and is intended to improve span-based reads, please add a unit test that readssection.BodyviaSpan<byte>(and ideally also via thebyte[]overload) to validate correct data/boundary behavior and guard against regressions.
public override int Read(byte[] buffer, int offset, int count)
=> Read(buffer.AsSpan(offset, count));
public override int Read(Span<byte> buffer)
{
if (_finished)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Read each section of a two part body into a span smaller than the section, with both the default buffer and one smaller than the body so the partial boundary match path is exercised as well.
Add Read(Span) override to MultipartReaderStream
Description
Move the synchronous read implementation onto Read(Span) and have the array overload delegate to it, so a caller reading a section into a span does not pay the base Stream fallback's rented array and copy. Also add the throwing Write(ReadOnlySpan) to match the other write overrides.